Group event notifications and inbound nukes in quick succession#4530
Group event notifications and inbound nukes in quick succession#4530blontd6 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds grouped and pluralized event notifications for repeated destroyed, captured, lost, and inbound nuke messages. Localization strings were expanded, EventsDisplay now aggregates matching events, and UnitImpl now emits structure-specific capture/loss messages and includes structures in delete-message display handling. ChangesEvent grouping feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant UnitImpl
participant EventsDisplay
participant addEvent
participant EventLog
UnitImpl->>EventsDisplay: emit unit_captured / unit_lost / unit_destroyed
EventsDisplay->>EventsDisplay: derive groupKey, unitType, targetPlayerName
EventsDisplay->>addEvent: pass event with grouping data
addEvent->>EventLog: search same groupKey within 30 ticks
alt match found
addEvent->>EventLog: increment count and refresh description
else no match
addEvent->>EventLog: append new row
end
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/hud/layers/EventsDisplay.ts (1)
37-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile string parsing with magic offsets.
parseInboundNukereverse-engineers the player name from hardcoded lengths (-20,-24,7,-22) of the exact English message. If any of these strings change (or the message is localized), the slice silently returns the wrong name ornull, and grouping quietly stops working. The MIRV offsets are especially error-prone because each⚠️is 2 UTF-16 units.Cleaner option: pass the nuke type and player name as structured fields on the incoming event so no string parsing is needed. If parsing must stay, at least compute offsets from the suffix constants instead of literals:
♻️ Derive offsets from the suffix strings
function parseInboundNuke(m: string): { player: string; nukeType: string } | null { - if (m.endsWith(" - atom bomb inbound")) return { player: m.slice(0, -20), nukeType: "atom" }; - if (m.endsWith(" - hydrogen bomb inbound")) return { player: m.slice(0, -24), nukeType: "hydrogen" }; - if (m.startsWith("⚠️⚠️⚠️ ") && m.endsWith(" - MIRV INBOUND ⚠️⚠️⚠️")) return { player: m.slice(7, -22), nukeType: "mirv" }; + const atom = " - atom bomb inbound"; + const hydrogen = " - hydrogen bomb inbound"; + const mirvPre = "⚠️⚠️⚠️ "; + const mirvSuf = " - MIRV INBOUND ⚠️⚠️⚠️"; + if (m.endsWith(atom)) return { player: m.slice(0, -atom.length), nukeType: "atom" }; + if (m.endsWith(hydrogen)) return { player: m.slice(0, -hydrogen.length), nukeType: "hydrogen" }; + if (m.startsWith(mirvPre) && m.endsWith(mirvSuf)) return { player: m.slice(mirvPre.length, -mirvSuf.length), nukeType: "mirv" }; return null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/hud/layers/EventsDisplay.ts` around lines 37 - 42, The parseInboundNuke helper is relying on fragile hardcoded slice offsets to recover the player name from event text. Update EventsDisplay.parseInboundNuke to avoid magic numbers by either passing structured player/nuke fields into the event handling path, or by deriving the start/end offsets from the exact suffix strings used in each message pattern. Keep the existing function and its MIRV branch, but make the parsing length-safe so changes to the message text or Unicode characters do not break grouping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/hud/layers/EventsDisplay.ts`:
- Line 35: The pluralizeUnit helper in EventsDisplay hardcodes English plural
forms, so update it to use the translated unit-name keys from the language
resources instead of deriving text from the raw enum string. Locate the
pluralization logic in pluralizeUnit and switch it to resolve the appropriate
unit.* entries from the i18n layer so event text stays locale-safe.
---
Nitpick comments:
In `@src/client/hud/layers/EventsDisplay.ts`:
- Around line 37-42: The parseInboundNuke helper is relying on fragile hardcoded
slice offsets to recover the player name from event text. Update
EventsDisplay.parseInboundNuke to avoid magic numbers by either passing
structured player/nuke fields into the event handling path, or by deriving the
start/end offsets from the exact suffix strings used in each message pattern.
Keep the existing function and its MIRV branch, but make the parsing length-safe
so changes to the message text or Unicode characters do not break grouping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24d4b3ef-03e5-4da4-a2fa-e002f60f946c
📒 Files selected for processing (3)
resources/lang/en.jsonsrc/client/hud/layers/EventsDisplay.tssrc/core/game/UnitImpl.ts
f25a559 to
e2fd466
Compare
|
looks like the CI is failing |
Add approved & assigned issue number here:
Resolves #4516
Description:
Restores real-time event notifications for structure capture, loss, and destruction (such as Cities, Ports, Silos, Factories, SAM Launchers, and Defense Posts), and aggregates these notifications alongside inbound atomic bombs, hydrogen bombs, and MIRVs when they occur in quick succession (within a 3-second / 30-tick window).
formats such as:
"Your 4 Cities were destroyed""Captured 4 Cities from [Player]""[Player] - 3 atom bombs inbound"This provides clear, concise, and immediate tactical feedback to players in high-stress combat moments without sacrificing important event tracking.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
blontd6